Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Keyboard interactions #76

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Conversation

royeden
Copy link

@royeden royeden commented Jan 17, 2023

Hi! Thanks for the cool library 😄

I was setting it up for a personal project and I noticed there weren't any keyboard interactions available, so I tried my best to set them up:

Changes:

  • Add dev command (runs rollup watch) so it's easier to debug this library in the future.
  • Reuse resolvedTransform:
// Before
const resolvedTransform = transform(); // Signal gets transform value
if (!transformsAreEqual(resolvedTransform, noopTransform())) { // It's compared
  const style = transformStyle(transform());  // Signal gets transform value again (this is unnecessary, as it should be the same transform)
  element.style.setProperty("transform", style.transform); // Set transform value
}

// After
const resolvedTransform = transform(); // Signal gets transform value and it's reused everywhere else
if (!transformsAreEqual(resolvedTransform, noopTransform())) {
  const style = transformStyle(resolvedTransform);
  element.style.setProperty("transform", style.transform as string);
}
  • Add support for keyboard sensors: Implemented a rudimentary keyboard sensor that tracks keydown events (only works if the element is focusable, so the examples should be updated to reflect on these changes).
  • Add support for transitions when using keyboard sensors: The library will only add transition: transform 0s when using pointers during the interaction. This allows the user to set transitions on the element for keyboard movement (inspired by dnd-kit).

I also accidentally ran into #28 when applying transitions to the elements and found a possible hack-ish solution around it for some cases (didn't get it to work with overlays, sadly, because they unmount before they can execute their transitions). Let me know if you would like to chat about it in another PR.

Caveats:

  • This PR doesn't address accessibility for the elements of the library (I'm willing to try and address this if I have some time in the following weeks).
  • There can be no interactions between sensors, and focusing of the element is currently not resetting the sensor state (this is also copyed from dnd-kit interactions).

* Reuse already computed transforms.
* Add support for keyboard sensors.
* Add support for transitions when using keyboard sensors.
@royeden
Copy link
Author

royeden commented Jan 17, 2023

Also leaving these questions / observations that I had on the code and couldn't quite understand while developing this feature:
*I don't know where they should be addressed (is it right to do it here, should it be on an issue?)

  • Why does a droppable support transforms? Is there anything that's setting them? How would they work, would they offset the container? Is this intended for nested support?

  • Why does the library reset transforms before firing the events that affect the placement of the elements on the DOM? How much control should the library have of the final placements of the elements?

For example, the hero that appears on the site with the dnd component is using

const onDragEnd = ({ draggable, droppable }) => {
    if (droppable) {
      const child = droppable.node.children[0];
      droppable.node.insertBefore(draggable.node, child);
      setInDropZone(true);
    } else if (draggable.node.parentElement !== draggablesContainer) {
      draggablesContainer.append(draggable.node);
      setInDropZone(false);
    }
  };

This is the interaction that sets the element inside of the dropzone. The issue with this approach is that before doing that it removes the transform on the draggable, so the element loses context of the previous action. This is another case where #28 comes into the conversation: losing context of the drag action state before computing the result of it can lead to some weird interactions. This is what I'm referring to as the hack-ish solution to transitions: I found the way around the order of the drop and the reset of the transforms and used the new order to recalculate the layout before resetting the transform on the draggable.

@martinpengellyphillips
Copy link
Contributor

Thanks for this - I'll take a look at it soon.

@martinpengellyphillips
Copy link
Contributor

I'll pick this up next now #80 is done. But in the meantime, I realised I hadn't answered your questions so here you go:

Why does a droppable support transforms? Is there anything that's setting them? How would they work, would they offset the container? Is this intended for nested support?

An example would be sortables. The droppables are transformed as part of the illusion of moving items. This needs to be distinct from draggable as the 'space' droppable for the dragged item needs to be positioned correctly as it moves.

Why does the library reset transforms before firing the events that affect the placement of the elements on the DOM? How much control should the library have of the final placements of the elements?

At the point onDragEnd etc fire, the drag is considered finished (the mouse has been released for example). I tried many iterations of deferring this to make things like animating to final position easier, but they ended up introducing more complexity and edge cases throughout the library. So I'll take a different approach now for #28

As an aside, the 'hero' example is outdated and a poor one as it manipulates the dom directly. I updated all the other examples to use Solid's reactive system more, but must have missed that one. It should be more like:

export const DragAndDropExample = () => {
  const [where, setWhere] = createSignal("outside");

  const onDragEnd: DragEventHandler = ({ droppable }) => {
    if (droppable) {
      setWhere("inside");
    } else {
      setWhere("outside");
    }
  };

  return (
    <DragDropProvider onDragEnd={onDragEnd}>
      <DragDropSensors />
      <div class="min-h-15">
        <Show when={where() === "outside"}>
          <Draggable />
        </Show>
      </div>
      <Droppable>
        <Show when={where() === "inside"}>
          <Draggable />
        </Show>
      </Droppable>
    </DragDropProvider>
  );
};

@royeden
Copy link
Author

royeden commented Feb 4, 2023

Thank you so much for the answers, they are absolutely clear and make a ton of sense!

I see that after the merge there's conflicts to solve, so I'll be doing that when I'm available either this weekend or this following week.

I've also checked a bit how dnd-kit does their aria attributes support and they use an announcement component that wraps their dnd container with a div that acts as an aria live region, where they display information on the current action inside of a node. It got me thinking on how to handle the same approach on this library, but I believe that for that to happen there's a need of the context of the conclusion of the action, therefore that would have to exist in userland, perhaps it can be achieved as another example on the site, so I'll check that in the future. If it can be generalized, it could be added to the package or as a separate dependency (although I believe accessibility is important and this is one of the few solutions for DND on solidjs, so I would go for adding the few extra bytes on the main package if it means that can be added).
In the meantime, I believe that we can simply add aria attributes to the dragged node indicating where it's positioned relative to it's original position and indicating if it's positioned on top of a droppable during sensor actions, perhaps that can be addressed as another issue and resolved in another PR.

There should also be a PR if this is merged to change the dragabbles from divs to buttons as they are focusable elements that will allow for keyboard interactions 😁

* Added intersection type to avoid unnecessary casts/checks
@royeden
Copy link
Author

royeden commented Feb 5, 2023

Fixed the conflicts!

@paxee
Copy link

paxee commented Aug 2, 2023

Hi guys, I tested this and discovered that after moving a draggable with the keyboard other keyboard interactions and things broke, for example input fields losing focus automatically. Apparently adding this check fixes the issue:

const clearSelection = () => {
    // window.getSelection()?.removeAllRanges();
    if (isActiveSensor()) {
      window.getSelection()?.removeAllRanges();
    }
  };

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants